Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch tests away from using enzyme.mount (components/higher-order/with-state/test/index.js) #7829

Conversation

nerrad
Copy link
Contributor

@nerrad nerrad commented Jul 9, 2018

Description

This switches all tests in components/higher-order/with-state/test/index.js from using enzyme.mount to React.TestUtils. This is because enzyme does not fully support React 16.3+ (and movement to do so is really slow). This will fix issues with breakage due to the enzyme incompatibility as components receive React 16.3+ features (such as forwardRef usage in #7557).

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

This switches all tests in `components/higher-order/with-state/test/index.js` from using enzyme.mount to `React.TestUtils`.  This is because `enzyme` does not fully support React 16.3+ (and movement to do so is really slow). This will fix issues with breakage due to the enzyme incompatibility as components receive React 16.3+ features (such as `forwardRef` usage in #7557).
@nerrad nerrad requested a review from aduth July 9, 2018 15:27
@nerrad nerrad self-assigned this Jul 9, 2018
@nerrad nerrad added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Refactoring labels Jul 9, 2018

// this is needed because TestUtils does not accept a stateless component.
// anything run through a HOC ends up as a stateless component.
const getTestComponent = ( WrappedComponent ) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using a similar HOC in other places should we implement it in a place where we can reuse it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:) Currently I've got pulls open for all the affected HOCs that implement this (or a variation of) when needed. I think there's definitely room for abstraction but I think that should be part of a larger discussion (via its own pull/issue) about creating our own abstraction for testing components. I'd like to focus on getting these pulls merged in first and then we can accumulate patterns that we notice across the various tests as a part of the discussion on the abstraction (I can contribute to that with patterns I noticed when doing these conversions).

Copy link
Contributor Author

@nerrad nerrad Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the current comment is currently against master branch incorrect. When/if #7557 gets implemented, then it will be completely correct because every HOC (created by createHigherOrderComponent will be wrapped by forwardRef).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using a similar HOC in other places should we implement it in a place where we can reuse it?

We definitely should do it once we merge all PRs :) I'd prefer to do it in one go so we large enough codebase to sparkle proper discussion.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@gziolo gziolo merged commit 322a3fc into master Jul 12, 2018
@gziolo gziolo added this to the 3.3 milestone Jul 12, 2018
@gziolo gziolo deleted the update/with-state.js-replace-usage-of-enzyme.mount-with-React.TestUtils branch July 12, 2018 12:37
@ellatrix ellatrix added [Type] Code Quality Issues or PRs that relate to code quality and removed [Type] Code Quality Issues or PRs that relate to code quality labels Nov 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants